Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Withdraw Instruction #15

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Withdraw Instruction #15

wants to merge 11 commits into from

Conversation

jdubpark
Copy link
Contributor

Closes #6

@jdubpark jdubpark self-assigned this Mar 10, 2022
Copy link
Member

@alecchendev alecchendev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for changing my account validation helpers to assert_msgs. General flow and logic of the instructions is on the right track. Nice one-liner for check_writable, rust 🤟 🦀

Some feedback:

  1. looks like you have a merge conflict still in .gitignore
  2. see Withdraw Instruction #15, i added a note about adding the payer subscription token account (need to check if the person withdrawing from deposit vault is the rightful owner of the subscription - i do a similar check in renew if you wanna see an example) - then also once you do this don't forget to update instruction.rs to reflect new account
  3. to address your note - you should be just fine letting the token program deal with verifying sufficient amount
  4. signer/owner of the deposit vault is actually the subscription metadata account - so in your instruction definition as well as the third account passed into the invoke_signed second parameter, the account will be your ctx.subscription_metadata_ai - means you'll also have to change the seeds to match that
    • when you actually look at the seeds, one of the seeds will be count, which atm you'll have to make the client pass in as an instruction parameter like i do in renew - i'm going to make a PR in like an hour or so where this count is stored in the subscription metadata so you can write your code as if that count param is stored in the subscription metadata

Great work so far and mad respect for taking the initiative to do some programming over break! As always, questions are welcome - just dm on discord

Copy link
Member

@alecchendev alecchendev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we talked yesterday but wanted to give more formal feedback:

  • The deposit vault is an associated token account (it's own PDA owned by the token program) of the subscription metadata PDA account, this means the token program requires a signature from this account in order to "authorize" a transfer. You already know to provide a signature for a PDA, you provide the seeds of that PDA to invoke_signed. The seeds for the subscription metadata PDA should be as follows
let subscription_seeds = &[
      b"subscription_metadata",
      payee.as_ref(),
      &amount.to_le_bytes(),
      &duration.to_le_bytes(),
      &count.to_le_bytes(),
      &[subscription_bump],
  ];

Other than that, from what we talked about over discord it seems like you've got everything for account validation (mainly validating signer/writable privileges, PDAs, checking token accounts' data if they're initialized (can use check_ata_initialized)), and then it's really just checking if the person withdrawing owns a token from the mint specified in the sub metadata.

@alecchendev
Copy link
Member

payer token account (mint_ai) doesn't need to be writable

beautiful verification of the PDAs and token account owners

        assert_msg(
            payer_token_account.mint == current_mint,
            SubscriptionError::InvalidSubscriptionOwner.into(),
            "Invalid subscription owner. Only the owner of a subscription associated with the deposit vault can withdraw.",
        )?;
        assert_msg(
            deposit_vault.mint == current_mint && deposit_vault.amount > 0,
            SubscriptionError::InvalidReceiver.into(),
            "Invalid receiver.",
        )?;
    }

second assert is checking wrong mint, deposit vault is just whatever token the subscription is being paid in, i.e. subscription.deposit_mint

assert_msg(
        payer_token_account.amount > 0,
        SubscriptionError::InsufficientWithdrawBalance.into(),
        "Insufficient funds to withdraw.",
    )?;

This is checking if the user owns a token of the subscription mint, i.e. that they own the subscription, just to clarify.

Everything else is perfect.

@alecchendev alecchendev marked this pull request as ready for review March 15, 2022 04:59
Copy link
Member

@alecchendev alecchendev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a couple swaps/reformatting things and renamed some things for clarity. Should be all good now.

Gonna wait until withdraw test script is done to merge this branch into master to ensure it's functioning properly - you (or whoever makes withdraw test script) can create a branch off of this one, merge that one into this branch, then merge this branch into main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Withdraw Instruction
2 participants